Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: Update cmake_minimum_required to 2.8.12 #3816

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

RytoEX
Copy link
Member

@RytoEX RytoEX commented Dec 2, 2020

Description

Bump CMake minimum version required to 2.8.12 for all subprojects.

Motivation and Context

CMake 3.19 has begun emitting a deprecation warning that support for CMake 2.8.12 will be dropped in a future CMake release.

CMake Deprecation Warning at deps/obs-scripting/obslua/CMakeLists.txt:1 (cmake_minimum_required):
Compatibility with CMake < 2.8.12 will be removed from a future version of
CMake.

Update the VERSION argument value or use a ... suffix to tell
CMake that the project does not need compatibility with older versions.

Warnings are bad.

The lowest minimum any subproject supported before this was 2.8, so bumping a patch version number shouldn't cause issues. Also, the OBS root CMakeLists.txt requires CMake 3.10, and 3.16 on Windows, so having the lower 2.x versions on the subprojects probably doesn't make much sense anyway, but I wanted to make a minimal change.

The only subproject I haven't updated is ftl-sdk because it's a git submodule. At some point, if we don't address this issue in ftl-sdk, the CMake support for compatibility with older than 2.8.12 will be removed. It's unclear if that means CMake will refuse to configure such projects, or if they just won't guarantee compatibility.

Currently, only YouNow is using the ftl-sdk as Mixer no longer exists and Restream dropped FTL support when Mixer went down.

How Has This Been Tested?

I built OBS on Windows 10 64-bit 2004 (Build 19041.630) before and after these changes. I noted the presence of the warning before and its absence after. I ran a quick recording session after to ensure that OBS worked as expected.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@WizardCM WizardCM added the Code Cleanup Non-breaking change which makes code smaller or more readable label Dec 2, 2020
@RytoEX
Copy link
Member Author

RytoEX commented Dec 2, 2020

Edited into the original comment, regarding ftl-sdk:
Currently, only YouNow is using the ftl-sdk as Mixer no longer exists and Restream dropped FTL support when Mixer went down.

@rsiv Is YouNow planning to continue using FTL and the ftl-sdk? Does YouNow have any interest in forking ftl-sdk to continue maintaining it?

@rsiv
Copy link
Contributor

rsiv commented Dec 2, 2020

Hey @RytoEX
Yes, we plan to continue using FTL and the ftl-sdk.
Let me check with our team regarding maintaining a fork.
Besides updating to a newer CMake, do you expect any other work to be required in the near term?

@RytoEX
Copy link
Member Author

RytoEX commented Dec 2, 2020

Hey @RytoEX
Yes, we plan to continue using FTL and the ftl-sdk.
Let me check with our team regarding maintaining a fork.
Besides updating to a newer CMake, do you expect any other work to be required in the near term?

@rsiv
I believe @notr1ch had previously submitted some issue reports to the https://github.com/mixer/ftl-sdk/ repo. He may be able to speak to those, but I'm not aware of anything else that we were seeking to modify. I came across the CMake issue and the mixer/ftl-sdk repo doesn't seem to be getting any updates, so we thought to reach out to you about it.

@rsiv
Copy link
Contributor

rsiv commented Dec 3, 2020

@RytoEX
I forked ftl-sdk and bumped CMake to 2.8.12: https://github.com/YouNow/ftl-sdk

@dori4n
Copy link

dori4n commented Dec 7, 2020

I have left the following comment in the developer discussion with the Glimesh folk, I'm going to leave it here, so you can see it, too:

It's fine to bump it up to 3.2 (I don't think anyone ever built it with something older, not even Beam/Mixer), the only real reason why it's at 2.8 is because OBS Studio was at the time (which they forked into their Tachyon software, which later became the OBS-FTL fork with the dedicated Beam Service in it). It was also the oldest version that could generate the build files for the compilers that can build the code, and to tell developers using it, that versions that old will be fine (If I recall correctly, not really sure right now). CMake will to the best of my knowledge only ever emit a warning for this, unless something else in the CMakeList is very off. Most things that break are usually the automated testing facilities, simple builds like used in the FTL SDK use directives that have remained the same throughout all of CMake's life and aren't likely to change anytime soon. I would avoid changing this, who cares about a little warning, if you know and understand what it means. You can suppress it, and add a comment about why you're suppressing it next to the directive that does. Personally, I prefer legacy support (as in having it work, not that there's any tech support call center or something like that), but 🤷‍♀️

@RytoEX
Copy link
Member Author

RytoEX commented Dec 7, 2020

Given that CMake recently broke configs using explicit LANGUAGE flags, I am not keen to simply ignore their warning that "Compatibility with CMake < 2.8.12 will be removed from a future version of CMake." The CMake devs have made clear that they are willing to break builds that rely on deprecated functionality or functionality being used outside of their designed use case. Furthermore, warning spam can drown out actually useful warnings, so we have an incentive to correct warnings when possible, rather than simply suppress or ignore them. I don't think I'm going to change my mind on whether or not we should make this change where we can.

I made the changes to all subprojects we control. When the matter of one we didn't (ftl-sdk and its subprojects/dependencies) came up, I discussed with the team and we decided that the best thing to do would be to reach out to YouNow about it, given that they were, at the time, the only known service still using or planning to use FTL. As mentioned on #3834, this also revived an internal discussion about whether to maintain or drop FTL support, which has briefly been discussed on that PR.

@dodgepong
Copy link
Member

Please direct further FTL discussion here: #4021

@jp9000 jp9000 merged commit 81c2dac into obsproject:master Jan 18, 2021
@RytoEX RytoEX deleted the update-cmake-version branch March 17, 2021 20:21
@RytoEX RytoEX added this to the OBS Studio 27.0 milestone Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Cleanup Non-breaking change which makes code smaller or more readable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants